-
-
Notifications
You must be signed in to change notification settings - Fork 346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dark theme for ConsoleUI #3226
Dark theme for ConsoleUI #3226
Conversation
Ok this is really cool! |
0c38bc0
to
b397f70
Compare
b397f70
to
13f8b42
Compare
Found an unrelated bug with the Console UI while testing this. Happens when leaving the path filed empty when adding a new game instance:
Fix should be as easy as this: diff --git a/ConsoleUI/GameInstanceScreen.cs b/ConsoleUI/GameInstanceScreen.cs
index d9f55bc4..555a0b1e 100644
--- a/ConsoleUI/GameInstanceScreen.cs
+++ b/ConsoleUI/GameInstanceScreen.cs
@@ -94,6 +94,12 @@ protected bool nameValid()
/// </summary>
protected bool pathValid()
{
+ if (string.IsNullOrEmpty(path.Value)) {
+ // Complain about empty path
+ RaiseError("Path cannot be empty!");
+ SetFocus(path);
+ return false;
+ }
if (Platform.IsMac) {
// Handle default path dragged-and-dropped onto Mac's Terminal
path.Value = path.Value.Replace("Kerbal\\ Space\\ Program", "Kerbal Space Program"); Do you want to add it to this PR? Otherwise I can do a separate one. Otherwise this looks good. I thought about adding a way to make the theme choice permanent so you don't have to add the argument yourself every time, and to help those launching the Console UI via the system menu entry from #3052. A new field in |
Interestingly, that exception doesn't conform to the .NET documentation; I'm thinking this should catch all exceptions and treat them as failure... |
13f8b42
to
7531a03
Compare
For a persistent setting, ConsoleUI currently has none of its own, and I'd prefer to keep it that way. EDIT: Now you should be able to affect the system menu entry by adding this to your environment (presumably export CKAN_CONSOLEUI_THEME=dark |
7531a03
to
ee772f4
Compare
Yeah, after writing the comment it came to me that we shouldn't put UI-specific settings into the core's Environment variables are a good idea. I kinda hoped that CommandlineParser would support them already, but doesn't look like it. There's an open PR for it though: commandlineparser/commandline#698 |
Same, this made me miss Python's |
Hold on, the "no such theme" message should print the valid options. Gimme a minute to add that... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works like a charm and looks very nice, even on Windows!
ee772f4
to
b18d242
Compare
Motivation
Currently ConsoleUI only supports one visual style, based on TuboVision.
Some users may prefer a less bright display.
Changes
Now ConsoleUI supports a
--theme
parameter accepting optionsdefault
(the default) anddark
. If you choosedark
, a monochrome green-on-black style is used:To achieve this,
Toolkit.ConsoleTheme
is changed from a singleton to a parameter that is passed to any spot that needs to draw things. AConsoleTheme.Themes
dictionary holds the defined themes, and new logic inConsoleCKAN
looks up the theme based on the--theme
parameter and passes it to the application code.